Add semantic focus cropping#46
Conversation
Greptile SummaryThis PR adds semantic focus cropping to
Confidence Score: 3/5The crop logic and detection plumbing are well-implemented, but unconditionally requiring the ML runtime stack in The implementation of
Important Files Changed
Reviews (2): Last reviewed commit: "chore: ignore local focus crop examples" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| protected function detectFocus(string $focus): array | ||
| { | ||
| self::$focusDetector ??= pipeline('zero-shot-object-detection'); | ||
|
|
||
| $path = tempnam(sys_get_temp_dir(), 'utopia-image-focus-'); | ||
| if ($path === false) { | ||
| throw new Exception('Failed to create image for focus detection'); | ||
| } | ||
|
|
||
| try { | ||
| if (file_put_contents($path, $this->image->getImageBlob(), LOCK_EX) === false) { | ||
| throw new Exception('Failed to create image for focus detection'); | ||
| } | ||
|
|
||
| $detections = (self::$focusDetector)($path, [$focus], threshold: 0.1, percentage: true, topK: PHP_INT_MAX); | ||
| } finally { | ||
| @unlink($path); | ||
| } |
There was a problem hiding this comment.
If pipeline() or the pipeline invocation throws (e.g., missing or incompatible ONNX Runtime, model not yet downloaded), the exception propagates straight through crop() and kills the entire image operation. Because the focus feature is additive and the caller already supplies a gravity fallback, users likely expect the method to fall back to gravity rather than crash. The PR notes this explicitly as a design choice, but it will cause unhandled exceptions for any caller on Alpine/musl or any environment where the binary artifacts have not been pre-downloaded, even if they are fine with a gravity-based result.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Image/Image.php
Line: 269-286
Comment:
**Hard failure when inference runtime is unavailable**
If `pipeline()` or the pipeline invocation throws (e.g., missing or incompatible ONNX Runtime, model not yet downloaded), the exception propagates straight through `crop()` and kills the entire image operation. Because the `focus` feature is additive and the caller already supplies a gravity fallback, users likely expect the method to fall back to gravity rather than crash. The PR notes this explicitly as a design choice, but it will cause unhandled exceptions for any caller on Alpine/musl or any environment where the binary artifacts have not been pre-downloaded, even if they are fine with a gravity-based result.
How can I resolve this? If you propose a fix, please make it concise.
| { | ||
| self::$focusDetector ??= pipeline('zero-shot-object-detection'); | ||
|
|
||
| $path = tempnam(sys_get_temp_dir(), 'utopia-image-focus-'); |
There was a problem hiding this comment.
tempnam prefix truncated on Windows
The prefix 'utopia-image-focus-' is 19 characters. PHP's tempnam() documentation states that on Windows only the first 3 characters of the prefix are used, so the created file gets the prefix uto instead. Keeping the prefix at 5 characters or fewer would work reliably on all platforms.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Image/Image.php
Line: 273
Comment:
**`tempnam` prefix truncated on Windows**
The prefix `'utopia-image-focus-'` is 19 characters. PHP's `tempnam()` documentation states that on Windows only the first 3 characters of the prefix are used, so the created file gets the prefix `uto` instead. Keeping the prefix at 5 characters or fewer would work reliably on all platforms.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
Summary
Image::crop()with an optional natural-languagefocusargumentExamples
Testing
./vendor/bin/phpunit --filter '/^(?!.*(avif|heic)).*$/'(51 tests, 282 assertions)composer lintcomposer checkcomposer validate --strictcomposer auditKnown Platform Constraint
TransformersPHP publishes glibc-linked Linux ONNX Runtime binaries. The existing Alpine/musl test image can build and run the non-inference suite with FFI enabled, but real model inference requires a musl-compatible ONNX Runtime build. This limitation is documented and inference failures are not silently converted to gravity fallback.
The three excluded full-suite tests are the existing local AVIF/HEIC encoder failures caused by unavailable/incompatible ImageMagick delegates.